-
Notifications
You must be signed in to change notification settings - Fork 12.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implementation of fmt::FormattingOptions
#118159
Conversation
r? @thomcc (rustbot has picked a reviewer for you, use r? to override) |
This comment has been minimized.
This comment has been minimized.
Looks like this broke some compiler tests. Will fix this evening, converting to draft PR for now. |
This comment has been minimized.
This comment has been minimized.
CI is green -> Converting back to "normal" (non-draft) PR. |
I already have a pretty big review backlog and haven't been following this change (so I'd have to read the ACP to figure out how it's supposed to behave), so I'm going to reroll for now, sorry. r? libs |
This comment has been minimized.
This comment has been minimized.
ec2787c
to
cc52c71
Compare
Fixed two more issues I've stumbled onto while playing around with the PR changes (see commit descriptions for details). I'll fix the PR description to represent the current API. |
☔ The latest upstream changes (presumably #116012) made this pull request unmergeable. Please resolve the merge conflicts. |
cc52c71
to
385944a
Compare
This comment was marked as resolved.
This comment was marked as resolved.
We haven't decided yet what to do with the "debug as hex" flags. It's fine to have them as unstable, but we shouldn't stabilize an interface for those flags without a separate discussion. I've added it as an unresolved question to the tracking issue. |
flags: u32, | ||
fill: char, | ||
align: rt::Alignment, | ||
width: Option<usize>, | ||
precision: Option<usize>, | ||
options: FormattingOptions, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes a Formatter
object bigger than before, because the flags
field is now stored in the new FormattingOptions as separate fields.
We should check if that doesn't have any negative impact on performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes a Formatter object bigger than before
Are you sure? Before, flags
was a u32
, so 4 bytes. With each flag in its own field, there are two bools and two small enums wrapped into Option
– both should be one byte per field, so 4 bytes in total as well. Unless I'm not seeing something here, both the original code and the new implementation have the same size.
However, the benchmark shows a performance regression, and when I realized that Formatter
didn't get bigger and therefore, flags
may not be the culprit, I was already halfway through refactoring FormattingOptions
to use a bitmask for flags
(like it did before). So, either way, let's test that, it might help even if a difference in size is not the cause of the performance regression. Could you please rerun the benchmark?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure?
No :)
Before, flags was a u32, so 4 bytes. With each flag in its own field, there are two bools and two small enums wrapped into Option – both should be one byte per field, so 4 bytes in total as well.
Good point!
I think that in the near future we might want to put more things in the fields flag though. At least the alignment can easiliy fit in there, and the Some/None flag of the width and precision could fit in there as well:
pub struct FormattingOptions {
flags: u32, // sign, zero pad, alt, debug-as-hex, width flag, precision flag, alignment
fill: char,
width: usize, // only used if width flag is set.
precision: usize, // only used if width flag is set.
}
But maybe that's overkill.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pub struct FormattingOptions { flags: u32, // sign, zero pad, alt, debug-as-hex, width flag, precision flag, alignment fill: char, width: usize, // only used if width flag is set. precision: usize, // only used if width flag is set. }
well, fill
has lots of spare bits, you could probably merge it with flags
, where flags
is just the upper bits and fill
is the lower 21 bits. though this would only be beneficial on <= 32
-bit systems unless width
and precision
were changed to be u32
or the struct was #[repr(packed(4))]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that there is definitely potential for further optimization here. I'll leave the solution using flags
– while it is a bit harder to read, the possible performance benefits in the future seem more important to me.
The benchmark still reports a small performance regression (~0.1 %). While I don't think this is a big problem, it would still be nice if this PR would not introduce any performance regressions. To make the code more maintainable, I changed all internal accesses to formatting options to use the getters/setters on FormattingOptions
instead of direct field access. I suspect this is at least a major factor in the performance regression. I have reverted this now, can you please rerun the benchmark @m-ou-se?
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
…<try> Implementation of `fmt::FormatttingOptions` Tracking issue: rust-lang#118117 Public API: ```rust #[derive(Copy, Clone, Debug, PartialEq, Eq)] pub struct FormattingOptions { … } #[derive(Copy, Clone, Debug, PartialEq, Eq)] pub enum Sign { Plus, Minus } #[derive(Copy, Clone, Debug, PartialEq, Eq)] pub enum DebugAsHex { Lower, Upper } impl FormattingOptions { pub fn new() -> Self; pub fn sign(&mut self, sign: Option<Sign>) -> &mut Self; pub fn sign_aware_zero_pad(&mut self, sign_aware_zero_pad: bool) -> &mut Self; pub fn alternate(&mut self, alternate: bool) -> &mut Self; pub fn fill(&mut self, fill: char) -> &mut Self; pub fn align(&mut self, alignment: Option<Alignment>) -> &mut Self; pub fn width(&mut self, width: Option<usize>) -> &mut Self; pub fn precision(&mut self, precision: Option<usize>) -> &mut Self; pub fn debug_as_hex(&mut self, debug_as_hex: Option<DebugAsHex>) -> &mut Self; pub fn get_sign(&self) -> Option<Sign>; pub fn get_sign_aware_zero_pad(&self) -> bool; pub fn get_alternate(&self) -> bool; pub fn get_fill(&self) -> char; pub fn get_align(&self) -> Option<Alignment>; pub fn get_width(&self) -> Option<usize>; pub fn get_precision(&self) -> Option<usize>; pub fn get_debug_as_hex(&self) -> Option<DebugAsHex>; pub fn create_formatter<'a>(self, write: &'a mut (dyn Write + 'a)) -> Formatter<'a>; } impl<'a> Formatter<'a> { pub fn new(write: &'a mut (dyn Write + 'a), options: FormattingOptions) -> Self; pub fn with_options<'b>(&'b mut self, options: FormattingOptions) -> Formatter<'b>; pub fn sign(&self) -> Option<Sign>; pub fn options(&self) -> FormattingOptions; } ``` Relevant changes from the public API in the tracking issue (I'm leaving out some stuff I consider obvious mistakes, like missing `#[derive(..)]`s and `pub` specifiers): - `enum DebugAsHex`/`FormattingOptions::debug_as_hex`/`FormattingOptions::get_debug_as_hex`: To support `{:x?}` as well as `{:X?}`. I had completely missed these options in the ACP. I'm open for any and all bikeshedding, not married to the name. - `fill`/`get_fill` now takes/returns `char` instead of `Option<char>`. This simply mirrors what `Formatter::fill` returns (with default being `' '`). - Changed `zero_pad`/`get_zero_pad` to `sign_aware_zero_pad`/`get_sign_aware_zero_pad`. This also mirrors `Formatter::sign_aware_zero_pad`. While I'm not a fan of this quite verbose name, I do believe that having the interface of `Formatter` and `FormattingOptions` be compatible is more important. - For the same reason, renamed `alignment`/`get_alignment` to `aling`/`get_align`. - Deviating from my initial idea, `Formatter::with_options` returns a `Formatter` which has the lifetime of the `self` reference as its generic lifetime parameter (in the original API spec, the generic lifetime of the returned `Formatter` was the generic lifetime used by `self` instead). Otherwise, one could construct two `Formatter`s that both mutably borrow the same underlying buffer, which would be unsound. This solution still has performance benefits over simply using `Formatter::new`, so I believe it is worthwhile to keep this method.
☀️ Try build successful - checks-actions |
See rust-lang#118159 (comment) for context. This reverts commit 62078df.
a2aaae7
to
31a5657
Compare
@bors try |
…<try> Implementation of `fmt::FormattingOptions` Tracking issue: rust-lang#118117 Public API: ```rust #[derive(Copy, Clone, Debug, PartialEq, Eq)] pub struct FormattingOptions { … } #[derive(Copy, Clone, Debug, PartialEq, Eq)] pub enum Sign { Plus, Minus } #[derive(Copy, Clone, Debug, PartialEq, Eq)] pub enum DebugAsHex { Lower, Upper } impl FormattingOptions { pub fn new() -> Self; pub fn sign(&mut self, sign: Option<Sign>) -> &mut Self; pub fn sign_aware_zero_pad(&mut self, sign_aware_zero_pad: bool) -> &mut Self; pub fn alternate(&mut self, alternate: bool) -> &mut Self; pub fn fill(&mut self, fill: char) -> &mut Self; pub fn align(&mut self, alignment: Option<Alignment>) -> &mut Self; pub fn width(&mut self, width: Option<usize>) -> &mut Self; pub fn precision(&mut self, precision: Option<usize>) -> &mut Self; pub fn debug_as_hex(&mut self, debug_as_hex: Option<DebugAsHex>) -> &mut Self; pub fn get_sign(&self) -> Option<Sign>; pub fn get_sign_aware_zero_pad(&self) -> bool; pub fn get_alternate(&self) -> bool; pub fn get_fill(&self) -> char; pub fn get_align(&self) -> Option<Alignment>; pub fn get_width(&self) -> Option<usize>; pub fn get_precision(&self) -> Option<usize>; pub fn get_debug_as_hex(&self) -> Option<DebugAsHex>; pub fn create_formatter<'a>(self, write: &'a mut (dyn Write + 'a)) -> Formatter<'a>; } impl<'a> Formatter<'a> { pub fn new(write: &'a mut (dyn Write + 'a), options: FormattingOptions) -> Self; pub fn with_options<'b>(&'b mut self, options: FormattingOptions) -> Formatter<'b>; pub fn sign(&self) -> Option<Sign>; pub fn options(&self) -> FormattingOptions; } ``` Relevant changes from the public API in the tracking issue (I'm leaving out some stuff I consider obvious mistakes, like missing `#[derive(..)]`s and `pub` specifiers): - `enum DebugAsHex`/`FormattingOptions::debug_as_hex`/`FormattingOptions::get_debug_as_hex`: To support `{:x?}` as well as `{:X?}`. I had completely missed these options in the ACP. I'm open for any and all bikeshedding, not married to the name. - `fill`/`get_fill` now takes/returns `char` instead of `Option<char>`. This simply mirrors what `Formatter::fill` returns (with default being `' '`). - Changed `zero_pad`/`get_zero_pad` to `sign_aware_zero_pad`/`get_sign_aware_zero_pad`. This also mirrors `Formatter::sign_aware_zero_pad`. While I'm not a fan of this quite verbose name, I do believe that having the interface of `Formatter` and `FormattingOptions` be compatible is more important. - For the same reason, renamed `alignment`/`get_alignment` to `aling`/`get_align`. - Deviating from my initial idea, `Formatter::with_options` returns a `Formatter` which has the lifetime of the `self` reference as its generic lifetime parameter (in the original API spec, the generic lifetime of the returned `Formatter` was the generic lifetime used by `self` instead). Otherwise, one could construct two `Formatter`s that both mutably borrow the same underlying buffer, which would be unsound. This solution still has performance benefits over simply using `Formatter::new`, so I believe it is worthwhile to keep this method.
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
Finished benchmarking commit (9180b8f): comparison URL. Overall result: ❌✅ regressions and improvements - no action neededBenchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf. @bors rollup=never Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary -0.4%, secondary 1.8%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeResults (primary 0.0%, secondary 0.1%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 770.008s -> 767.823s (-0.28%) |
@bors r+ |
☀️ Test successful - checks-actions |
Finished benchmarking commit (75716b4): comparison URL. Overall result: ❌✅ regressions and improvements - please read the text belowOur benchmarks found a performance regression caused by this PR. Next Steps:
@rustbot label: +perf-regression Instruction countThis is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.
Max RSS (memory usage)Results (primary 0.3%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesResults (secondary 3.4%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Binary sizeResults (primary 0.1%, secondary 0.1%)This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
Bootstrap: 769.893s -> 768.721s (-0.15%) |
Marking the perf regression as triaged. It doesn't seem to affect enough benchmarks to merit further investigation, especially given that this is an intentional change to the API surface (which seems likely to at least shift around e.g. CGUs and such and cause some amount of noise). |
Thanks for getting this merged and triaged! Also, sorry for not reacting to the performance regression notification sooner. |
Tracking issue: #118117
Public API:
Relevant changes from the public API in the tracking issue (I'm leaving out some stuff I consider obvious mistakes, like missing
#[derive(..)]
s andpub
specifiers):enum DebugAsHex
/FormattingOptions::debug_as_hex
/FormattingOptions::get_debug_as_hex
: To support{:x?}
as well as{:X?}
. I had completely missed these options in the ACP. I'm open for any and all bikeshedding, not married to the name.fill
/get_fill
now takes/returnschar
instead ofOption<char>
. This simply mirrors whatFormatter::fill
returns (with default being' '
).zero_pad
/get_zero_pad
tosign_aware_zero_pad
/get_sign_aware_zero_pad
. This also mirrorsFormatter::sign_aware_zero_pad
. While I'm not a fan of this quite verbose name, I do believe that having the interface ofFormatter
andFormattingOptions
be compatible is more important.alignment
/get_alignment
toaling
/get_align
.Formatter::with_options
returns aFormatter
which has the lifetime of theself
reference as its generic lifetime parameter (in the original API spec, the generic lifetime of the returnedFormatter
was the generic lifetime used byself
instead). Otherwise, one could construct twoFormatter
s that both mutably borrow the same underlying buffer, which would be unsound. This solution still has performance benefits over simply usingFormatter::new
, so I believe it is worthwhile to keep this method.